Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PTFE-672: api authentication #322

Merged
merged 19 commits into from
Aug 11, 2023
Merged

PTFE-672: api authentication #322

merged 19 commits into from
Aug 11, 2023

Conversation

Abubakarr99
Copy link
Contributor

No description provided.

tcarmet
tcarmet reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@tcarmet tcarmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how I see it according to the example.

runner_manager/main.py Outdated Show resolved Hide resolved
runner_manager/main.py Outdated Show resolved Hide resolved
@Abubakarr99 Abubakarr99 marked this pull request as ready for review August 7, 2023 09:02
@Abubakarr99 Abubakarr99 requested a review from a team as a code owner August 7, 2023 09:02
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2023 •
edited
Loading

Codecov Report

Merging #322 (088a8b0) into main (260129b) will increase coverage by 0.78%.
The diff coverage is 98.00%.

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   90.27%   91.06%   +0.78%     
==========================================
  Files          17       21       +4     
  Lines         360      403      +43     
==========================================
+ Hits          325      367      +42     
- Misses         35       36       +1
Flag Coverage Δ
api 32.00% <98.00%> (+7.84%) ⬆️
unit 70.22% <6.00%> (-7.84%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
runner_manager/auth.py 95.45% <95.45%> (ø)
runner_manager/main.py 80.00% <100.00%> (+3.52%) ⬆️
runner_manager/models/settings.py 100.00% <100.00%> (ø)
runner_manager/routers/_health.py 100.00% <100.00%> (ø)
runner_manager/routers/private.py 100.00% <100.00%> (ø)
runner_manager/routers/public.py 100.00% <100.00%> (ø)

@tcarmet tcarmet changed the title PTFE-693: api authentication PTFE-672: api authentication Aug 7, 2023
tcarmet
tcarmet reviewed Aug 7, 2023
View reviewed changes
tests/api/tests_auth.py Outdated Show resolved Hide resolved
runner_manager/main.py Outdated Show resolved Hide resolved
runner_manager/main.py Outdated Show resolved Hide resolved
@tcarmet tcarmet marked this pull request as draft August 9, 2023 00:33
@tcarmet tcarmet force-pushed the feature/PTFE-693-unit-test-settings-1 branch from d4430bc to c814f6f Compare August 9, 2023 02:48
tcarmet
tcarmet requested changes Aug 9, 2023
View reviewed changes
runner_manager/main.py Outdated Show resolved Hide resolved
@Abubakarr99 Abubakarr99 marked this pull request as ready for review August 9, 2023 07:26
tcarmet
tcarmet approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@tcarmet tcarmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It LGTM, left one task that is harder to do speaking about routes, try your best on it, if it doesn't work out of the box leave it as is and we'll re-iterate on it as needed.

runner_manager/routers/_health.py Outdated Show resolved Hide resolved
runner_manager/auth.py Outdated Show resolved Hide resolved
runner_manager/auth.py Outdated Show resolved Hide resolved
runner_manager/main.py Outdated Show resolved Hide resolved
@Abubakarr99 Abubakarr99 merged commit e3eb51c into main Aug 11, 2023
10 of 11 checks passed
@Abubakarr99 Abubakarr99 deleted the feature/PTFE-693-unit-test-settings-1 branch August 11, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcarmet tcarmet tcarmet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Abubakarr99 @codecov-commenter @tcarmet